Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract frequency management methods of Event into trait #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kstkn
Copy link
Collaborator

@kstkn kstkn commented Mar 29, 2020

Putting helper method into separate trait we reduce reasons for Event class to be changed and make it smaller

Fixes #57

@kstkn kstkn requested a review from omnilight March 29, 2020 12:59
@yuniorsk
Copy link

Hi, is there any chance to merge this one? I would really appreciate it, thanks a lot 🤞

@darealfive
Copy link

@yuniorsk please could you explain why it's necessary to put those methods into a trait?
Sure, the classfile "Event" itself gets smaller and but not the loaded class as it uses that trait.

Additionally you need to check both classes, Event and ManagesFrequencies to get a picture of the class Event.

Furthermore for me it does not seem to be "good practice" to let the trait access the Event property $this->_expression which is part of the Event and not of the trait. Not a "good practice", cause you rely on the fact that all classes using the trait needs a property "_expression" in order to work (instead of defining an abstract method within the trait "abstract protected function getExpression()“ to require a kind of an interface for the trait).

Please do not get me wrong. I am a bit skeptical when it comes to traits as they easily can mess up things in big projects.

@yuniorsk
Copy link

@darealfive I am only interested in missing methods like hourlyAt etc. I don't mind if it's extracted into trait or not.

@yuniorsk
Copy link

I have already cheered for these missing api methods here #41, but with no luck 🤷‍♂️ If there some way I can help with it just let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port ManagesFrequencies trait
3 participants